Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: data versioning #2068

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

feat: data versioning #2068

wants to merge 24 commits into from

Conversation

PiTrem
Copy link
Member

@PiTrem PiTrem commented Aug 12, 2024

See #1253.

@PiTrem PiTrem marked this pull request as draft August 12, 2024 09:43
@@ -0,0 +1,98 @@
require 'open-uri'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing frozen string literal comment.

app/api/modules/logidze_module.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
class ContainerHierarchy < ApplicationRecord

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing frozen string literal comment.

app/services/versioning/fetcher.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,60 @@
# frozen_string_literal: true

class Versioning::Fetchers::ReactionFetcher

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

app/services/versioning/fetchers/sample_fetcher.rb Outdated Show resolved Hide resolved
app/services/versioning/fetchers/sample_fetcher.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,34 @@
# frozen_string_literal: true

class Versioning::Fetchers::ScreenFetcher

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

new(**args).call
end

def call

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/AbcSize: Assignment Branch Condition size for call is too high. [<14, 33, 4> 36.07/25]

versions += Versioning::Serializers::ContainerSerializer.call(analysis, ["Analysis: #{analysis.name}"])

analysis.children.with_deleted.with_log_data.each do |dataset|
versions += Versioning::Serializers::ContainerSerializer.call(dataset, ["Analysis: #{analysis.name}", "Dataset: #{dataset.name}"])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/LineLength: Line is too long. [138/120]

versions += Versioning::Serializers::ContainerSerializer.call(dataset, ["Analysis: #{analysis.name}", "Dataset: #{dataset.name}"])

dataset.attachments.with_log_data.each do |attachment|
versions += Versioning::Serializers::AttachmentSerializer.call(attachment, ["Analysis: #{analysis.name}", "Dataset: #{dataset.name}", "Attachment: #{attachment.filename}"])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/LineLength: Line is too long. [182/120]

end

screen.research_plans.each do |research_plan|
versions += Versioning::Fetchers::ResearchPlanFetcher.call(research_plan: research_plan, prefix: "Research Plan: #{research_plan.name}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/LineLength: Line is too long. [142/120]

@@ -0,0 +1,38 @@
# frozen_string_literal: true

class Versioning::Merger

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.


merged_versions = []
grouped_versions.each_with_index do |(_, v), index|
changes = v.map do |v|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/ShadowingOuterLocalVariable: Shadowing outer local variable - v.

app/services/versioning/reverter.rb Outdated Show resolved Hide resolved
app/services/versioning/serializers/base_serializer.rb Outdated Show resolved Hide resolved
app/services/versioning/serializers/base_serializer.rb Outdated Show resolved Hide resolved
app/services/versioning/serializers/base_serializer.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,90 @@
# frozen_string_literal: true

class Versioning::Serializers::ContainerSerializer < Versioning::Serializers::BaseSerializer

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

new(record: record, name: name).call
end

def field_definitions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/AbcSize: Assignment Branch Condition size for field_definitions is too high. [<14, 31, 5> 34.38/25]

@headri headri marked this pull request as ready for review September 26, 2024 14:37
Copy link
Collaborator

@JanCBrammer JanCBrammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a few suggestions to revert the changes to semantically unchanged files.
This would make the PR a bit smaller which would make working with the PR in the GitHub UI a bit easier. Atm, the UI is quite unresponsive due to to its size (large diffs in many files).

I'll continue the review over the coming days.

app/api/chemotion/sample_api.rb Outdated Show resolved Hide resolved
app/assets/stylesheets/sample-details.scss Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this stub?

app/models/reactions_product_sample.rb Outdated Show resolved Hide resolved
app/models/sample.rb Show resolved Hide resolved
public/klasses.json Outdated Show resolved Hide resolved
public/jsmol Outdated Show resolved Hide resolved
app/packs/src/stores/alt/actions/ElementActions.js Outdated Show resolved Hide resolved
import ScreensFetcher from 'src/fetchers/ScreensFetcher';
import WellplatesFetcher from 'src/fetchers/WellplatesFetcher';

export default class VersionsTable extends Component {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe convert this to a functional component? I feel like introducing new components might be an opportunity to move the codebase away from class components.

app/packs/src/apps/mydb/elements/details/VersionsTable.js Outdated Show resolved Hide resolved
const alertText = revertibleFields() < 0
? 'You cannot undo changes. You have unsaved data which would be lost.'
: `You cannot undo these changes.
Either the changes are up to date, it is the first version or all changes are irreversible.`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user perspective it would be great if this alert would be more specific, i.e., the three cases that are lumped together in the fallback of the ternary could be differentiated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the formatting changes, otherwise this file's diff is unnecessarily hard to review.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the formatting changes, otherwise this file's diff is unnecessarily hard to review.

app/packs/src/fetchers/SamplesFetcher.js Outdated Show resolved Hide resolved
app/packs/src/models/ResearchPlan.js Outdated Show resolved Hide resolved
app/packs/src/models/Sample.js Outdated Show resolved Hide resolved
@@ -112,45 +113,59 @@ class Sample < ApplicationRecord
pg_search_scope :search_by_cas, against: { xref: 'cas' }

# scopes for suggestions
scope :by_residues_custom_info, ->(info, val) { joins(:residues).where("residues.custom_info -> '#{info}' ILIKE ?", "%#{sanitize_sql_like(val)}%")}
scope :by_residues_custom_info, lambda { |info, val|
joins(:residues).where("residues.custom_info -> '#{info}' ILIKE ?", "%#{sanitize_sql_like(val)}%")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/LineLength: Line is too long. [134/120]

scope :by_name, ->(query) { where('name ILIKE ?', "%#{sanitize_sql_like(query)}%") }
scope :by_sample_xref_cas,
->(query) { where("xref ? 'cas'").where("xref ->> 'cas' ILIKE ?", "%#{sanitize_sql_like(query)}%") }
scope :by_exact_name, ->(query) { where('lower(name) ~* lower(?) or lower(external_label) ~* lower(?)', "^([a-zA-Z0-9]+-)?#{sanitize_sql_like(query)}(-?[a-zA-Z])$", "^([a-zA-Z0-9]+-)?#{sanitize_sql_like(query)}(-?[a-zA-Z])$") }
scope :by_exact_name, lambda { |query|
where('lower(name) ~* lower(?) or lower(external_label) ~* lower(?)', "^([a-zA-Z0-9]+-)?#{sanitize_sql_like(query)}(-?[a-zA-Z])$", "^([a-zA-Z0-9]+-)?#{sanitize_sql_like(query)}(-?[a-zA-Z])$")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/LineLength: Line is too long. [217/120]

scope :by_reaction_solvent_ids, ->(ids) { joins(:reactions_solvent_samples).where('reactions_samples.reaction_id in (?)', ids) }
scope :by_reaction_ids, ->(ids) { joins(:reactions_samples).where('reactions_samples.reaction_id in (?)', ids) }
scope :by_reaction_reactant_ids, lambda { |ids|
joins(:reactions_reactant_samples).where('reactions_samples.reaction_id in (?)', ids)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails/WhereEquals: Use where(reactions_samples: { reaction_id: ids }) instead of manually constructing SQL.

scope :by_reaction_solvent_ids, ->(ids) { joins(:reactions_solvent_samples).where('reactions_samples.reaction_id in (?)', ids) }
scope :by_reaction_ids, ->(ids) { joins(:reactions_samples).where('reactions_samples.reaction_id in (?)', ids) }
scope :by_reaction_reactant_ids, lambda { |ids|
joins(:reactions_reactant_samples).where('reactions_samples.reaction_id in (?)', ids)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/LineLength: Line is too long. [122/120]

joins(:reactions_reactant_samples).where('reactions_samples.reaction_id in (?)', ids)
}
scope :by_reaction_product_ids, lambda { |ids|
joins(:reactions_product_samples).where('reactions_samples.reaction_id in (?)', ids)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails/WhereEquals: Use where(reactions_samples: { reaction_id: ids }) instead of manually constructing SQL.

self.creator.increment_counter 'samples'
return unless /^#{creator.name_abbreviation}-\d+$/.match?(short_label)

creator.increment_counter 'samples'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails/SkipsModelValidations: Avoid using increment_counter because it skips validations.

@@ -132,7 +133,7 @@ def create_subwellplate(user, collection_ids, _copy_ea = false)
readouts: w.readouts,
additive: w.additive,
position_x: w.position_x,
position_y: w.position_y
position_y: w.position_y,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/AbcSize: Assignment Branch Condition size for create_subwellplate is too high. [<11, 30, 4> 32.2/25]

@@ -132,7 +133,7 @@ def create_subwellplate(user, collection_ids, _copy_ea = false)
readouts: w.readouts,
additive: w.additive,
position_x: w.position_x,
position_y: w.position_y
position_y: w.position_y,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/MethodLength: Method has too many lines. [31/30]

end

def call
changes.each do |change|
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/BlockLength: Block has too many lines. [26/25]

@@ -0,0 +1,7 @@
# frozen_string_literal: true

class Versioning::Reverters::WellReverter < Versioning::Reverters::BaseReverter
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

@@ -0,0 +1,7 @@
# frozen_string_literal: true

class Versioning::Reverters::WellplateReverter < Versioning::Reverters::BaseReverter
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

new(record: record, name: name).call
end

def field_definitions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/MethodLength: Method has too many lines. [73/30]

JSON.parse(result).join("\n")
},
revert: %i[extended_metadata.hyperlinks],
revertible_value_formatter: ->(key, value) { JSON.parse(jsonb_formatter('hyperlinks').call(key, value) || '[]') },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/LineLength: Line is too long. [124/120]

@@ -0,0 +1,33 @@
# frozen_string_literal: true

class Versioning::Serializers::ElementalCompositionSerializer < Versioning::Serializers::BaseSerializer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

@@ -0,0 +1,88 @@
# frozen_string_literal: true

class Versioning::Serializers::ReactionSerializer < Versioning::Serializers::BaseSerializer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

end
end

def lookups
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/PerceivedComplexity: Perceived complexity for lookups is too high. [10/8]

@@ -0,0 +1,68 @@
# frozen_string_literal: true

class Versioning::Serializers::ResidueSerializer < Versioning::Serializers::BaseSerializer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

new(record: record, name: name).call
end

def field_definitions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/MethodLength: Method has too many lines. [44/30]

@@ -0,0 +1,41 @@
# frozen_string_literal: true

class Versioning::Serializers::ScreenSerializer < Versioning::Serializers::BaseSerializer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

new(record: record, name: name).call
end

def field_definitions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/MethodLength: Method has too many lines. [31/30]

const isRevertible = revertibleFields();
let alertText = 'You cannot undo changes. ';
if (isRevertible === 0) {
alertText += 'Every change is either up to date or irreversible';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the first and last conditional branch both contain the "all changes are irreversible" case.

Copy link

LCOV of commit 7c8ac70 during Continuous Integration #4020

Summary coverage rate:
  lines......: 61.6% (14094 of 22897 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

@headri headri force-pushed the versioning branch 2 times, most recently from afdcee9 to 89dd152 Compare October 22, 2024 13:38
Copy link

LCOV of commit 89dd152 during Continuous Integration #4048

Summary coverage rate:
  lines......: 61.6% (14081 of 22876 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

Copy link
Collaborator

@JanCBrammer JanCBrammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature needs tests 🧐

@PiTrem PiTrem changed the title Versioning feat: data versioning Nov 7, 2024
@PiTrem PiTrem marked this pull request as draft November 7, 2024 10:38
@PiTrem PiTrem added this to the v2.0.0 milestone Nov 7, 2024
Copy link
Collaborator

@JanCBrammer JanCBrammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look ok. Nice that there's at least some basic coverage now.

mschneider85 and others added 10 commits November 8, 2024 08:52
Store a history which keeps track of every change made to certain
entities and add a "History" tab to the Reaction/Sample detail pages.
Related entities are displayed in the version history. E.g. Reaction ->
ReactionsSample

Save timestamp and author of the version with the changes. A change diff
to the previous version can be displayed.

Tracked entities:

- Attachment

- Container

- ElementalComposition

- Reaction

- ReactionsSample

- Residue

- Sample

Co-authored-by: VadimKeller <[email protected]>
Add revert button to versioned field

reset colum information

re-create schema

update solvent label
Make revert function work (#780)

- Update version list after reverting a change

- Fix issue /w big svgs

- Make reverting molfile / svg work

- Fix merge issues

- Replace passing local variables /w using state

Add missing versioning for Screens, Wellplates, and Research plans

Change the way versions are displayed in the version table. Previously
we had one version per database update. Now we group versions by the
request id. These changes are now merged to make it more intuitive for
the user.

Implement revert functionality /w fetchers, reverters, and serializers

Implement a new revert UI with one button per version and one modal to
select revertible values. Delete old revert button.

Co-authored-by: Martin Schneider <[email protected]>
Co-authored-by: VadimKeller <[email protected]>
@headri headri marked this pull request as ready for review November 8, 2024 09:15
Copy link

github-actions bot commented Nov 8, 2024

LCOV of commit ab6c437 during Continuous Integration #4128

Summary coverage rate:
  lines......: 64.8% (14411 of 22227 lines)
  functions..: no data found
  branches...: no data found

Files changed coverage rate: n/a

@PiTrem PiTrem mentioned this pull request Nov 12, 2024
10 tasks
Copy link
Collaborator

@JanCBrammer JanCBrammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the latest implementation in the UI. I deleted a starting material from a reaction and saved the reactions. That resulted in two observations that puzzle me a bit:

  1. I'd expect the deleted starting material to show up in the "previous version" column, and be absent in the "updated version" column. Not the other way around (like on the screenshot):
    Screenshot from 2024-11-28 09-12-09

  2. Although I have saved the reaction, a warning indicates that I cannot undo the change due to unsaved changes:
    Screenshot from 2024-11-28 09-09-59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants